Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(vue): incorrect view rendered when using router.go(-n) #29877

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

bentleyo
Copy link
Contributor

@bentleyo bentleyo commented Sep 17, 2024

resolves #28201

This PR fixes the navigation issue related to router.go that was identified in issue #28201. After working on this issue I realised that @xxllxhdj has already created a PR for this in #29847. While their fix is great, I have added tests to replicate the issue, reused existing code and undefined will be returned in unexpected situations - which matches the existing functionality.

What is the current behavior?

If a user navigates from /home -> /pageA -> /pageB -> /pageC -> back to /pageB -> then router.go(-2) is called the URL will be updated to /home correctly, but the app will try to render /pageA.

This happens for any delta < -1.

What is the new behavior?

The app will correctly render /pageA, which matches the URL.

Does this introduce a breaking change?

  • Yes
  • No

@bentleyo bentleyo requested a review from a team as a code owner September 17, 2024 09:03
Copy link

vercel bot commented Sep 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ionic-framework ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 22, 2024 8:18pm

@bentleyo
Copy link
Contributor Author

The main problem was that when using findLastLocation with deltas less than -1, the code no longer checked relative to the specified route. My code change fixes this 😄

@bentleyo
Copy link
Contributor Author

@brandyscarney just a heads up, this PR relates to one assigned to you here: #29877

@bentleyo
Copy link
Contributor Author

bentleyo commented Oct 9, 2024

@thetaPC are you able to check this one for me and let me know if any changes required on my end to get this one through? 🙏

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for submitting a PR!

I noticed you removed the if statement from the code. I'd love to understand your reasoning behind that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @thetaPC thanks for the review!

The if statement I've removed was originally added to handle the situation where delta is less than -1. In that situation what it does is return the location delta many steps back from the end of the history stack.

if (delta < -1) {
  return locationHistory[locationHistory.length - 1 + delta];
}

However, this wasn't correct behaviour. There is a routeInfo passed into the function and the returned location should be delta steps back from that. Not back from the end of locationHistory.

This is why the bug reported in the issue was happening.

I've made it so it handles both situations using that same code. This is mostly reusing the code that was in the else statement.

That code essentially searches backwards through the history stack until it finds routeInfo and returns its parent (handling the delta -1 situation). I realised I could use index of that location in the array + delta to return the correct location.

I've added tests to handle the situations and ensure the behaviour works as expected. Let me know if you need any more details :)

@bentleyo bentleyo requested a review from thetaPC October 14, 2024 01:25
Co-authored-by: xxllxhdj <[email protected]>
Copy link
Contributor

@thetaPC thetaPC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@thetaPC thetaPC added this pull request to the merge queue Oct 22, 2024
@thetaPC thetaPC removed this pull request from the merge queue due to a manual request Oct 22, 2024
@thetaPC
Copy link
Contributor

thetaPC commented Oct 22, 2024

Gave @xxllxhdj co author credit as the team decided to go with this approach. Thank you everyone!

@thetaPC thetaPC added this pull request to the merge queue Oct 22, 2024
Merged via the queue into ionic-team:main with commit e32fbe0 Oct 22, 2024
49 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: vue @ionic/vue package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: router.go() not working as expected in ionic vue app
2 participants